-
Notifications
You must be signed in to change notification settings - Fork 434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add InstantClick behavior #1101
Add InstantClick behavior #1101
Conversation
Because it is not only triggered on mouseover, but could also be on mousedown, or eventually touchstart.
Since turbo-prefetch-cache-time is set to 1 millisecond in the html fixture
anchor_ prefix is used for all anchors in the tests
This looks awesome—thanks for your work on it!
Regarding the name, let's consider what ideas are conveyed by the related words:
One contains the verb "load" and the other contains "fetch", implying that preloading and prefetching perform different operations. Both contain "pre" which is a temporal reference, suggesting doing something early, where "early" means before some other significant point in time. Since both have the same temporal reference, it would imply that they both perform their operations at the same early point in time. However the truth is the opposite:
Therefore it would be vastly preferable in my opinion if they shared the verb "load" but the new one had a different way of referring to when it performs this loading operation. The clearest option I can think of is "hoverload". It loads the linked page on hover. I think this will avoid lots of confusion when reading code, where we have to stop and ask ourselves "wait, what's the difference between preloading and prefetching again?" |
@brandondrew Thanks for chiming in! Agree that the naming might lead to confusion and I like Although if we base the name on the default behavior, which is hovering, and think of touching the screen as the equivalent of "hovering" over a link on a mobile device, then it does make sense. Happy to make the change, though would like to wait for a few more folks to chip in too! |
hmm... you're right, it might not be perfect.... but I can't think of anything better so far... 😄
Maybe someone else will have a better idea... |
|
"PredictiveLoad"? |
Thanks for this proposal that would be a great addition. I agree that the proposed naming is confusing with the current preload attribute. What about |
Could we build on top of the existing data attribute?
I am not sure if that would make the implementation much more complex, but I think if it was built around the existing data attribute, it would avoid a lot of confusion. Both features preload the links, the difference is when and how, but for most people that are going to use this feature, the only thing that will really matter is when they want to preload the link, and having different data attributes will make that confusing vs same data attribute with different values |
I think it is also worth mentioning that similar behavior has been suggested in the original PR that introduced |
@davidalejandroaguilar thanks for bring this up, it is something we definitely want to add to Turbo! I'm going to give it a spin in one of our apps and and will come back with feedback after that. |
Okay, this suggestion has a lot of merit, and in many ways makes sense (I like the cohesive API suggested) but I have strong reservations about falling back to preloading everything on pageload for clients that can't handle loading on hover. To determine whether the question is moot or not, do we know that there are actually any browsers in this situation? Would there actually be people who have to have everything load on page load because their browser can't handle the code in this PR that's been (at some future date) added to Turbo? If there are no such browsers, then I like this suggestion a lot. But if there are browsers like that, then I would not want to be forced to make those browsers fall back to preloading everything on pageload. Imagine a gallery home page with 100 links that all go to image-heavy pages. Imagine a user on a slow cellular network opening the gallery home page. |
If the attribute is added to each link, then why does this section (below) seem to imply that it is controlled at the page level, and the links are opt-out only?
...
Did I misunderstand that 👆? IMHO it would be preferable if the developer could also opt-in at the element level. |
As far as names (if it is given a different name instead of a new value of the preload attribute) I think @adrienpoly's suggestion might be the catchiest name, and I like it a lot. It describes the user's experience (more or less) but it doesn't give a very clear description to a developer of what's really happening. @airblade's suggestion is the most accurate description of what it does, but (Also, My 2¢ in summary: |
I didn't mean falling back to pageload, what I meant is that I think if someone specifies |
A slightly different - but in my opinion way simpler solution in Turbo 'core' - would be to change the existing Implementing this would allow developers to add their own behavior (ie. "instant click", preload only links above the fold, ..) by simply dynamically adding a The behavior described in this PR (hover, viewport, ..) would be very well suited as a separate add-on/package, providing reasonable default behavior as described in the PR description. This PR introduces code that is almost duplicate of the existing preloading behavior. There's already a PR (#911) changing the preloader to use |
I think this would lead to unexpected results and is a breaking change, as least when combined with the Turbo preloaded links, where the behavior is that Turbo displays the preloaded snapshot from the cache while performing a fetch for the url. In our application, we use the Turbo preload pattern quite extensively - all preloads fetch "skeleton pages" (that are extremely cachable with no database queries made and with a This ensures that tapping a link navigates instantly and displays a skeleton page, and then the full page with dynamic and user-specific content is fetched by Turbo. This PR assumes that the page returned by a prefetch request is the same as a full normally fetched page, which I think is not always the case as described above and is a breaking change from how Turbo behaves today. I also believe the existing preloading behavior of Turbo supports not re-fetching the preloaded page, by setting cache headers (as the subsequent fetch request would be a cache hit in the browser) if that is what you'd want. |
Come to think of it, we are actually also doing "insta click" behavior with Turbo today, although it misuses the internal I've added a gist here for inspiration: https://gist.github.com/pfeiffer/d53bd40a39ee0586f54303e525c60fe2 |
I'm wondering why this would be needed? Why not rely on normal HTTP caching mechanism and headers to communicate TTL and let |
@guillaumebriday I like the idea 👍 We don't need it for this PR, but it's something we can add later. |
…tclick-behavior * origin/main: Keep Trix dynamic styles in the head (hotwired#1133)
I'm OK with that. The menu is omakase but substitutions are still possible. |
Great work @davidalejandroaguilar 👏 |
@afcapel Thank you! And thanks again for your time. Excited for all the awesomeness that's coming for everyone on Turbo 8! 🚀 I'll open a follow-up PR for the configurable delay and another one for the connection health check. |
Nice! Can you release a new beta so we can try it on our apps? 🚀 |
@guillaumebriday I've just released v8.0.0.beta.3! |
I strongly agree with this |
* origin/main: Introduce `turbo:{before-,}morph-{element,attribute}` events Turbo 8.0.0-beta.4 Introduce data-turbo-track="dynamic" (hotwired#1140) Ensure that the turbo-frame header is not sent when the turbo-frame has a target of _top (hotwired#1138) Turbo 8.0.0-beta.3 Fix attribute name (hotwired#1134) Add InstantClick behavior (hotwired#1101) Revert hotwired#926. (hotwired#1126) Keep Trix dynamic styles in the head (hotwired#1133) Remove unused stylesheets when navigating (hotwired#1128) Upgrade idiomorph to 0.3.0 (hotwired#1122) Debounce page refreshes triggered via Turbo streams Update copyright year to 2024 (hotwired#1118) Turbo 8.0.0-beta.2 Set aria-busy on the form element during a form submission (hotwired#1110) Dispatch `turbo:before-fetch-{request,response}` during preloading (hotwired#1034)
Is it possible to implement caching to prevent multiple requests for a single link? Currently, every time I hover over the same link, a new request is made. Why isn't this request cached? If the concern is that cached data might be outdated, we could consider making a second request upon click. Alternatively, is it better to use a delay? However, if the user moves the mouse over the link too quickly and clicks, wouldn't this result in no request being made? |
@Kagayakashi Hey there! 👋🏼 The initial implementation cached requests internally for a configurable duration of 10 seconds. However, during testing with a big app like Basecamp, @afcapel noticed this approach could lead to stale content, particularly in areas using older Javascript without Turbo. So for now, we opted to prioritize compatibility over optimization. Despite this, we're still able to harness the benefits of HTTP caching! Regarding the delay, it was added to prevent a flurry of requests when casually scrolling over a list of links. There are demo videos showing a before and after for this on the PR description. |
Why disable it globally, instead of making it configurable?
Why not do that by making the default configuration as compatible as possible?
Are you referring to caching by proxy servers? By the browser, by default? |
Is there any chance to configure this to work like this?
The cache could be configured longer then, e.g. 1 minute or so, because there will be always a new fetch after visiting the cached link. The current default behaviour fetches the same page multiple times, just scrolling an moving the mouse over the a page. Is this really a good default choice? I know the discussion was lengthy already, but maybe it can be reconsidered? (For my case I will simply disable it, but I'd prefer to have a solution that works like above with one cached fetch and a second fetch on page visit.) |
This is a bizarre default! |
Just flagging that that the PR says:
However, the code says:
Which suggests that this feature is actually enabled by default. It's no great effort to add the appropriate |
* Move doesNotTargetIFrame to util.js * Move findLinkFromClickTarget to util.js * Move getLocationForLink to util.js * Allow request to be intercepted and overriden on turbo:before-fetch-request * Add instantclick behavior * Allow customizing the event that triggers prefetching * Allow customizing the cache time for prefetching * Rename LinkPrefetchOnMouseoverObserver to LinkPrefetchObserver Because it is not only triggered on mouseover, but could also be on mousedown, or eventually touchstart. * Use private methods in LinkPrefetchObserver * Reorganize methods on LinkPrefetchObserver * Require a shorter sleep time in the test Since turbo-prefetch-cache-time is set to 1 millisecond in the html fixture * Standardize anchor IDs in link_prefetch_observer_tests anchor_ prefix is used for all anchors in the tests * Don't try traverse DOM to determine if the target is a link This is not necessary, since we can just check if the target is an anchor element with an href attribute. We were just using findLinkFromClickTarget because it had the selector we needed, but we can just use the selector directly. * Keep the closing tag on the same line as the rest of the tag * Remove unnecessary nesting in tests * Add missing newline at end of file * Check for prefetch meta tag before prefetching (on hover event) * Use FetchRequest to build request for LinkPrefetchObserver * LinkPrefetchObserver implements the FetchRequest interface, so it can be used to build a request. * It also adds this.response to FetchRequest to store the non-awaited `fetch` response, because we need to FetchRequest#receive() a `fetch` response, not a FetchRequest. * Add Turbo Stream header to Accept header when link has data-turbo-stream * Bring back prefetching links with inner elements * Add cancelable delay to prefetching links on hover * Fix clearing cache on every prefetch after b9e82f2 * Add tests for the delay on the meta tag * Use mouseenter and mouseleave instead of mouseover and mouseout To avoid having to traverse the DOM to find the link element * Remove unneeded comment * Use double quotes instead of single quotes for consistency * Move link variable declaration inside if statement Since target is only a link if isLink is true * Use correct key name for mouseenter event on LinkPrefetchObserver.triggerEvents On 5078e0b we started using the `mouseenter` event instead of the `mouseover` event to trigger prefetching. However, we forgot to update the key name on the `LinkPrefetchObserver.triggerEvents` object. * Allow prefetching when visiting page without meta, then visiting one with it * Allow create and delete posts with comments on the test server * Clear prefetch cache after form submission * Add test for nested data-turbo-prefetch=true within data-turbo-prefetch=false * No longer allow customizing the prefetch trigger event * No longer allow customizing the prefetch delay * Add touchstart event to prefetch observer * Fix flaky tests This commit fixes the flaky tests by ensuring that each worker has its own database file. This is done by adding a `worker_id` query parameter to the URLs of the pages that are being tested. This `worker_id` is passed to the database functions, which then use it to determine the name of the database file. It's necessary because the tests are running in parallel, and the database file is shared between all the workers. This means that if one worker creates a post, the other workers will see that post, and the tests will fail. * Use double quotes instead of single quotes * Only cache the link you're currently hovering Instead of maintaining a cache of all the links that have been hovered in the last 10 seconds. This solves issues where the user hovers a link, then performs a non-safe action and then later clicks the link. In this case, we would be showing stale content from before the action was performed. * Remove unused files after ETA template rendered removal * Remove unused variable * Clear prefetch cache when the link is no longer hovered This avoids a flurry of requests when casually scrolling down a page * Style changes --------- Co-authored-by: Alberto Fernández-Capel <[email protected]>
@jarvis-cochrane It was enabled by default in a subsequent PR #1162! |
Thank you! |
Description
This PR adds InstantClick-like behavior to Turbo. For those not familiar with it, what it does is prefetch links that are likely to be clicked on.
The result is effectively instant navigation in most cases.
Implementation
mouseover
.fetch
request to prefetch it and save the request withoutawait
ing for it to a cache with an expiration of 10 seconds (configurable).touchstart
.turbo:before-fetch-request
.fetch
request out of it and put it on theevent.detail.response
.FetchRequest#perform
method will now accept overriding the response internally (not exposed as part of public API) by assigning it toevent.detail.response
on theturbo:before-fetch-request
event added by the observer.fetch
request.await
ed and#receive
d as usual (most probably by this time, thefetch
request promise has already resolved), resulting in an instant navigation.Configuration
The amount of time a link is prefetched again after hovering can be configured via a
<meta name="turbo-prefetch-cache-time" content="1000">
tag, in milliseconds, and defaults to10000
.This behavior is disabled by default and can be opted-in via a
<meta name="turbo-prefetch" content="true">
tag.You can opt-out a link from being prefetched on hover by adding a
data-turbo-prefetch="false"
attribute to it.You can also opt-out/in descendants of an element this way (opt-out/in by container).
Notes
data-turbo-preload
on specific links we want to preload on page load.fetch
request sequentially in order to manually populate theSnapshotCache
, whereas the prefetched ones are notawait
ed when created, and thus allows for non-blocking concurrent execution of requests.SnapshotCache
like preloading does, but just lets Turbo do everything automatically by just using the cachedfetch
request instead of creating a new one. This means it's less intrusive on the internals of Turbo.Prior art
In no specific order:
touchstart
, and have been running in production for a while with no hiccups.Demo
A scaffold Rails app with 0.5 delay on the
PostsController#show
action.Noticeable delay:
Screen.Recording.2023-12-05.at.10.08.44.p.m.mov
Instant navigation:
Screen.Recording.2023-12-05.at.10.06.39.p.m.mov
Scrolling down a page with many links
Screen.Recording.2023-12-09.at.2.41.43.a.m.mov
Screen.Recording.2023-12-09.at.2.42.49.a.m.mov